cpp/src/mip_heuristics/utils.cuh (1)

342-344: Consider replacing the magic 1e-6 with a named constant or a tolerance parameter.

The hardcoded 1e-6 here is inconsistent with the rest of the codebase, which derives near-zero thresholds from solver-configured tolerances (e.g., tols.absolute_tolerance passed through get_cstr_tolerance). Callers have no way to adjust this threshold for problems with very small or very large objective scales — exactly the situation this PR is addressing with scaling.

♻️ Suggested refactor

+static constexpr double compute_rel_mip_gap_zero_tol = 1e-6;
+
 template <typename f_t>
-f_t compute_rel_mip_gap(f_t user_obj, f_t solution_bound)
+f_t compute_rel_mip_gap(f_t user_obj, f_t solution_bound, f_t zero_tol = static_cast<f_t>(compute_rel_mip_gap_zero_tol))
 {
-  if (integer_equal<f_t>(user_obj, 0.0, 1e-6)) {
-    return integer_equal<f_t>(solution_bound, 0.0, 1e-6) ? 0.0
-                                                         : std::numeric_limits<f_t>::infinity();
+  if (integer_equal<f_t>(user_obj, f_t{0}, zero_tol)) {
+    return integer_equal<f_t>(solution_bound, f_t{0}, zero_tol) ? f_t{0}
+                                                                 : std::numeric_limits<f_t>::infinity();
   }
   return std::abs(user_obj - solution_bound) / std::abs(user_obj);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cpp/src/mip_heuristics/utils.cuh` around lines 342 - 344, Replace the
hardcoded 1e-6 near-zero threshold in the integer_equal checks with a named
tolerance or configurable parameter: stop using the literal in the
integer_equal<f_t>(user_obj, 0.0, 1e-6) and integer_equal<f_t>(solution_bound,
0.0, 1e-6) calls and instead pass a named constant (e.g., near_zero_tolerance)
or the solver-configured tolerance obtained from
get_cstr_tolerance/tols.absolute_tolerance supplied into the function; update
the function signature or call sites around integer_equal usage so callers can
control the tolerance for user_obj and solution_bound comparisons.

cpp/src/pdlp/solve.cu (1)

1001-1007: Avoid unconditional dual-simplex conversion in inside-MIP concurrent mode.

dual_simplex_problem is constructed even when settings.inside_mip is true and the dual-simplex thread is skipped. That conversion can be deferred to the non-MIP branch to reduce setup overhead.

♻️ Suggested refactor sketch

-  dual_simplex::user_problem_t<i_t, f_t> dual_simplex_problem =
-    cuopt_problem_to_simplex_problem<i_t, f_t>(problem.handle_ptr, problem);
+  std::unique_ptr<dual_simplex::user_problem_t<i_t, f_t>> dual_simplex_problem_ptr;
+  if (!settings.inside_mip) {
+    dual_simplex_problem_ptr = std::make_unique<dual_simplex::user_problem_t<i_t, f_t>>(
+      cuopt_problem_to_simplex_problem<i_t, f_t>(problem.handle_ptr, problem));
+  }

   dual_simplex::user_problem_t<i_t, f_t> barrier_problem =
     settings.inside_mip
       ? cuopt_problem_to_simplex_problem<i_t, f_t>(problem.handle_ptr, *concurrent_problem)
-      : dual_simplex_problem;
+      : *dual_simplex_problem_ptr;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cpp/src/pdlp/solve.cu` around lines 1001 - 1007, Currently
dual_simplex_problem is always constructed via cuopt_problem_to_simplex_problem
even when settings.inside_mip is true and the dual-simplex thread is skipped;
change the logic so you only call cuopt_problem_to_simplex_problem to create
dual_simplex_problem inside the non-MIP branch and, when settings.inside_mip is
true, set barrier_problem from cuopt_problem_to_simplex_problem(...,
*concurrent_problem) without constructing dual_simplex_problem. Update the block
that defines dual_simplex_problem and barrier_problem (referencing
dual_simplex_problem, barrier_problem, cuopt_problem_to_simplex_problem,
settings.inside_mip, concurrent_problem) to defer or eliminate the unconditional
conversion and only perform the conversion needed for the chosen branch.

cpp/src/mip_heuristics/solve.cu (1)

142-149: Feasibility OR (is_feasible_after_unscaling || is_feasible_before_scaling) biases toward accepting solutions.

This means a solution that is feasible only in the scaled space but not in the original problem space will still be reported as feasible to the user. While the warning at Line 143 alerts the user about a mismatch, the downstream consumer of mip_solution_t will see a feasible status regardless of which space the solution is actually feasible in.

This is a reasonable pragmatic choice for MIP heuristics (scaling-induced numerical drift shouldn't reject an otherwise good solution), but consider whether reporting a NumericalIssues flag alongside the feasible solution would give users more diagnostic visibility.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cpp/src/mip_heuristics/solve.cu` around lines 142 - 149, The current call to
scaled_sol.get_solution uses (is_feasible_after_unscaling ||
is_feasible_before_scaling) which can mark a solution feasible even if it only
holds in the scaled space; change this so the reported feasibility reflects the
original problem (use is_feasible_after_unscaling for the primary feasible flag)
and add a diagnostic flag on the returned mip_solution_t (e.g., NumericalIssues
or scaling_mismatch) when is_feasible_before_scaling !=
is_feasible_after_unscaling; update the call site in scaled_sol.get_solution and
the mip_solution_t construction/definition to accept and propagate this
diagnostic so consumers see both the canonical feasibility and any
scaling-related numerical issue.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

cpp/src/mip_heuristics/mip_scaling_strategy.cu (1)

260-263: row_skip_scaling is now correctly zero-initialized before use.

thrust::fill at Lines 261-262 ensures row_skip_scaling is deterministic before compute_big_m_skip_rows populates it. This resolves the prior uninitialized-read risk.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cpp/src/mip_heuristics/mip_scaling_strategy.cu` around lines 260 - 263, The
prior uninitialized-read risk for row_skip_scaling is fixed by explicitly
zero-initializing it; ensure you keep the thrust::fill call that uses
handle_ptr_->get_thrust_policy() to fill row_skip_scaling.begin()..end() with
i_t(0) before calling compute_big_m_skip_rows so compute_big_m_skip_rows only
reads initialized values from row_skip_scaling; confirm the allocation of
iteration_scaling remains unchanged and placed after row_skip_scaling is
initialized.

cpp/src/mip_heuristics/mip_scaling_strategy.cu (2)

66-80: Prefer thrust::maximum<T> / thrust::minimum<T> over custom max_op_t / min_op_t.

Both structs are direct re-implementations of standard Thrust functors that are already __host__ __device__-compatible and accepted by CUB's DeviceSegmentedReduce::Reduce.

♻️ Proposed refactor

-template <typename t_t>
-struct max_op_t {
-  __host__ __device__ t_t operator()(const t_t& lhs, const t_t& rhs) const
-  {
-    return lhs > rhs ? lhs : rhs;
-  }
-};
-
-template <typename t_t>
-struct min_op_t {
-  __host__ __device__ t_t operator()(const t_t& lhs, const t_t& rhs) const
-  {
-    return lhs < rhs ? lhs : rhs;
-  }
-};

Then replace usages with thrust::maximum<f_t>{} and thrust::minimum<f_t>{} at call sites.

As per coding guidelines: "Avoid reinventing functionality already available in Thrust, CCCL, or RMM libraries."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cpp/src/mip_heuristics/mip_scaling_strategy.cu` around lines 66 - 80, Replace
the custom functors max_op_t and min_op_t with the standard Thrust functors:
remove or stop using max_op_t and min_op_t and update all call sites to use
thrust::maximum<f_t>{} and thrust::minimum<f_t>{} (or the appropriate template
type instead of f_t) so the code leverages the existing __host__
__device__-compatible Thrust implementations accepted by
CUB::DeviceSegmentedReduce::Reduce.

---

104-177: compute_big_m_skip_rows recomputes row_inf_norm redundantly, and ignores the passed temp_storage stream.

Two issues:

1. The function hard-codes op_problem.handle_ptr->get_stream() for its CUB calls (unlike compute_row_inf_norm which accepts a stream_view parameter). This inconsistency would silently break if a caller ever passes a different stream.
2. The max-abs segmented reduction on Lines 121-130 recomputes row_inf_norm that is already computed in the first call to compute_row_inf_norm on loop iteration 0 (Line 301). Since compute_big_m_skip_rows is called once before the loop, row_inf_norm gets computed twice before any scaling is applied.

Consider adding a stream_view parameter to compute_big_m_skip_rows consistent with compute_row_inf_norm, and passing the already-computed row_inf_norm from a preceding compute_row_inf_norm call instead of re-running the max reduce.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cpp/src/mip_heuristics/mip_scaling_strategy.cu` around lines 104 - 177,
compute_big_m_skip_rows currently reruns the max-abs segmented reduction into
row_inf_norm and hard-codes op_problem.handle_ptr->get_stream(), causing
redundant work and a stream mismatch; change the signature of
compute_big_m_skip_rows to accept a raft::stream_view (or stream_view alias used
elsewhere) and remove the initial max-abs cub::DeviceSegmentedReduce call so the
function uses the row_inf_norm that was produced by compute_row_inf_norm; also
replace all uses of op_problem.handle_ptr->get_stream() in
compute_big_m_skip_rows with the new stream_view and ensure
temp_storage/temp_storage_bytes and other CUB/Thrust calls use that stream_view
when invoked; finally update callers to compute_big_m_skip_rows to pass the
stream_view and to call compute_row_inf_norm once beforehand so row_inf_norm is
not recomputed.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro